-
Notifications
You must be signed in to change notification settings - Fork 13k
chore: improve the way we send DDP connection data to hooks #38268 #38294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughServer event flow refactored to emit strongly-typed, sau-prefixed events (login/logout/socket) with explicit payload fields (userAgent, loginToken, clientAddress, instanceId, connectionId, host). SAUMonitor and device-management consume these payloads and parse UA strings directly. A new getHeader utility safely extracts header values. Changes
Sequence Diagram(s)sequenceDiagram
participant Hooks as Hooks (sauMonitorHooks)
participant SauService as sau/service
participant SAUMonitor as SAUMonitor
participant DeviceMgmt as Device Management
Hooks->>SauService: emit sau.accounts.login {userId, instanceId, connectionId, loginToken?, clientAddress, userAgent, host}
SauService->>SAUMonitor: forward sau.accounts.login payload
SAUMonitor->>SAUMonitor: _getUserAgentInfo(userAgent) -> device info
SAUMonitor->>SAUMonitor: _handleSession({userId,...}, params) -> persist/update session
Hooks->>SauService: emit sau.socket.disconnected {instanceId, connectionId}
SauService->>SAUMonitor: forward sau.socket.disconnected
SAUMonitor->>SAUMonitor: finish sessions for connectionId
SauService->>DeviceMgmt: emit device-login {userId, userAgent, loginToken?, clientAddress}
DeviceMgmt->>DeviceMgmt: parse UA, send notifications/emails
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/statistics/server/lib/SAUMonitor.ts (1)
135-142: Update logout log messages to the new event name.The warning strings still mention
'accounts.logout', which no longer matches the hook name and can mislead debugging.🔧 Suggested fix
- if (!userId) { - logger.warn({ msg: "Received 'accounts.logout' event without 'userId'" }); + if (!userId) { + logger.warn({ msg: "Received 'sau.accounts.logout' event without 'userId'" }); return; } if (!sessionId) { - logger.warn({ msg: "Received 'accounts.logout' event without 'sessionId'" }); + logger.warn({ msg: "Received 'sau.accounts.logout' event without 'sessionId'" }); return; }
🤖 Fix all issues with AI agents
In `@packages/core-typings/src/sau/SocketConnectedPayload.ts`:
- Around line 1-3: Remove the inline comment inside the SocketConnectedPayload
type and replace the placeholder with an explicit empty payload type;
specifically, update export type SocketConnectedPayload to an explicit empty
shape (e.g., use Record<string, never> or an empty object type) so the intent is
encoded in the type rather than a comment and the inline comment is removed.
🧹 Nitpick comments (3)
apps/meteor/server/services/sauMonitor/service.ts (1)
35-38: Remove commented-out debug statement.Per coding guidelines, avoid code comments in the implementation. This commented-out console.log should be removed.
Suggested fix
this.onEvent('socket.connected', async (data) => { - // console.log('socket.connected', data); sauEvents.emit('sau.socket.connected', data); });apps/meteor/server/services/device-management/service.ts (1)
14-19: Address TODO: Add loginToken to payload.The TODO indicates
loginTokenshould be added to the device-login payload. TheDeviceLoginPayloadtype already supports an optionalloginTokenfield (as seen insession.tsusage at line 42).Would you like me to help implement the
loginTokenextraction similar to how it's done insauMonitorHooks.ts(usingAccounts._hashLoginToken(resume))? Or should I open an issue to track this task?apps/meteor/server/hooks/sauMonitorHooks.ts (1)
80-86: Remove inline comment and clarifySocketConnectedPayloadintent.The comment on line 82 should be removed per coding guidelines. Additionally, spreading the entire
connectionobject intoSocketConnectedPayloadappears to contradict the PR's goal of avoiding passing full connection objects downstream. Consider extracting only the required fields.Suggested fix
If only specific fields are needed, extract them explicitly:
Meteor.onConnection((connection) => { const socketConnectedEventObject: SocketConnectedPayload = { - // Implement SocketConnectedPayload type in case of using the sau.socket.connected hook - ...connection, + connectionId: connection.id, + // Add other required fields as needed }; sauEvents.emit('sau.socket.connected', socketConnectedEventObject); });If the full connection is intentionally needed here, the comment should at minimum be removed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38294 +/- ##
===========================================
+ Coverage 70.77% 70.78% +0.01%
===========================================
Files 3159 3160 +1
Lines 109384 109420 +36
Branches 19644 19669 +25
===========================================
+ Hits 77412 77457 +45
+ Misses 29939 29933 -6
+ Partials 2033 2030 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/statistics/server/lib/SAUMonitor.ts (1)
235-245: Replace the inline TODO block with an explicit UA result type.The comment block violates the "no implementation comments" guideline. Move the type information into code as a type alias instead.
However, the proposed solution using
ReturnType<typeof UAParserMobile.uaObject>won't work as expected sinceUAParserCustom.jsis untyped JavaScript. Define the union type explicitly instead:♻️ Suggested approach
+ type UAResult = + | UAParser.IResult + | { device?: { type: string }; app?: { name: string; version: string }; browser?: undefined; os?: undefined } + | { device?: { type: string }; os?: Record<string, string>; app?: { name: string; version: string }; browser?: undefined }; + - const result = ((): any => { + const result = ((): UAResult => {The types correspond to the three code branches: standard UAParser, mobile app, and desktop app respectively.
♻️ Duplicate comments (1)
packages/core-typings/src/sau/SocketConnectedPayload.ts (1)
1-2: Remove inline comment; encode empty payload in the type.Line 1 violates the “no implementation comments” guideline; encode the empty payload via the type instead. As per coding guidelines, avoid code comments in implementation.
♻️ Proposed change
-export type SocketConnectedPayload = {}; +export type SocketConnectedPayload = Record<string, never>;Is `Record<string, never>` the recommended TypeScript pattern for expressing an empty object payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 14 files
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/services/sauMonitor/events.ts`:
- Around line 4-12: The event type for 'sau.accounts.login' is inconsistent with
ISocketConnection because connection.loginToken is optional; update the event
definition in events.ts to make loginToken optional (change loginToken: string
to loginToken?: string) so it matches ISocketConnection and avoids the type
contract violation, then recompile/typecheck; alternatively, if you prefer
forcing a string, update the caller in service.ts where connection.loginToken is
passed to provide a default (e.g., connection.loginToken ?? '')—pick one
approach and apply it consistently across sau.accounts.login, service.ts, and
references in sauMonitorHooks.ts.
🧹 Nitpick comments (3)
apps/meteor/server/services/sauMonitor/service.ts (1)
42-45: Remove commented-out debug code.The commented
console.logstatement should be removed to keep the codebase clean.♻️ Proposed fix
this.onEvent('socket.connected', async (data) => { - // console.log('socket.connected', data); sauEvents.emit('sau.socket.connected', { instanceId: InstanceStatus.id(), connectionId: data.id }); });apps/meteor/server/hooks/sauMonitorHooks.ts (1)
49-58: Consider consolidating the twoMeteor.onConnectionhandlers.Both handlers register on the same connection event. Merging them would reduce overhead and improve code organization.
♻️ Proposed consolidation
Meteor.onConnection((connection) => { + // in case of implementing a listener of this event, define the parameters type in services/sauMonitor/events.ts + sauEvents.emit('sau.socket.connected', { instanceId: InstanceStatus.id(), connectionId: connection.id }); + connection.onClose(async () => { sauEvents.emit('sau.socket.disconnected', { connectionId: connection.id, instanceId: InstanceStatus.id() }); }); }); - -Meteor.onConnection((connection) => { - // in case of implementing a listener of this event, define the parameters type in services/sauMonitor/events.ts - sauEvents.emit('sau.socket.connected', { instanceId: InstanceStatus.id(), connectionId: connection.id }); -});apps/meteor/app/statistics/server/lib/SAUMonitor.ts (1)
238-307: Return type includesundefinedbut function always returns a value.The function always returns
{ device: info }(lines 304-306), but the return type is{ device: ISessionDevice } | undefined. This could lead to unnecessary null checks by callers.♻️ Proposed fix
- private _getUserAgentInfo(uaString: string): { device: ISessionDevice } | undefined { + private _getUserAgentInfo(uaString: string): { device: ISessionDevice } {
… into chore/ddp-headers
… into chore/ddp-headers
|
@coderabbitai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 8 files
✅ Actions performedReview triggered.
|
apps/meteor/server/lib/getHeader.ts
Outdated
| @@ -0,0 +1,11 @@ | |||
| export const getHeader = (headers: unknown, key: string): string => { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be in the tools package.
It would also be nice to have unit tests for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also use some typeguards https://www.typescriptlang.org/docs/handbook/advanced-types.html
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/server/hooks/sauMonitorHooks.ts`:
- Line 22: Replace the private Meteor call Accounts._hashLoginToken with the
public hashLoginToken from `@rocket.chat/account-utils`: import { hashLoginToken }
and change the expression that computes loginToken (currently using
Accounts._hashLoginToken(resume)) to use hashLoginToken(resume) (preserving the
ternary behavior with resume). Update any existing imports to add hashLoginToken
and remove reliance on Accounts._hashLoginToken so the code computes loginToken
via the public API.
- Line 38: Remove the duplicate device-login emission in sauMonitorHooks by
deleting the deviceManagementEvents.emit('device-login', { userId:
info.user._id, userAgent, loginToken, clientAddress }) call, and instead update
DeviceManagementService (the service that listens to accounts.login and emits
'device-login') to include the loginToken in its emitted payload so the single
emission covers the session handler in
apps/meteor/ee/server/lib/deviceManagement/session.ts; ensure the emitted object
from DeviceManagementService contains userId, userAgent, clientAddress and
loginToken to match the original payload.
🧹 Nitpick comments (1)
packages/tools/src/getHeader.ts (1)
3-3: Consider exportingHeaderLiketype for consumers.Consumers of
getHeadermight benefit from having access to theHeaderLiketype alias for their own type annotations. This is optional since the function's parameter typing provides guidance.♻️ Optional change
-type HeaderLike = IncomingHttpHeaders | Record<string, string | string[] | undefined>; +export type HeaderLike = IncomingHttpHeaders | Record<string, string | string[] | undefined>;
… into chore/ddp-headers
2f1df1c to
2a47751
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/tools/src/getHeader.ts">
<violation number="1" location="packages/tools/src/getHeader.ts:5">
P1: `getHeader` now uses a generic return type with `as T` casts, but it does not guarantee the runtime value matches `T` (e.g., `T=string[]` still returns a string for `Headers`, and the default mode can now return `string[]` for Node headers). This is both a type-safety issue and a potential runtime regression for callers that expect a string.</violation>
</file>
<file name="apps/meteor/server/hooks/sauMonitorHooks.ts">
<violation number="1" location="apps/meteor/server/hooks/sauMonitorHooks.ts:24">
P2: `clientAddress` is a required `string`, so it may be `''` when unknown. Using `??` won’t fall back to `x-real-ip` in that case (behavior change from the previous `getClientAddress` helper which used `||`). Use `||` to preserve the fallback when `clientAddress` is an empty string.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| export const getHeader = <T extends string | string[] = string>(headers: HeaderLike, key: string): T => { | ||
| if (!headers) { | ||
| return '' as T; | ||
| } | ||
|
|
||
| if (headers instanceof Headers) { | ||
| return (headers.get(key) ?? '') as T; | ||
| } | ||
|
|
||
| return (headers[key] ?? '') as T; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: getHeader now uses a generic return type with as T casts, but it does not guarantee the runtime value matches T (e.g., T=string[] still returns a string for Headers, and the default mode can now return string[] for Node headers). This is both a type-safety issue and a potential runtime regression for callers that expect a string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tools/src/getHeader.ts, line 5:
<comment>`getHeader` now uses a generic return type with `as T` casts, but it does not guarantee the runtime value matches `T` (e.g., `T=string[]` still returns a string for `Headers`, and the default mode can now return `string[]` for Node headers). This is both a type-safety issue and a potential runtime regression for callers that expect a string.</comment>
<file context>
@@ -2,34 +2,14 @@ import type { IncomingHttpHeaders } from 'http';
-export function getHeader(headers: HeaderLike, key: string, asArray: true): string[];
-
-export function getHeader(headers: HeaderLike, key: string, asArray = false): string | string[] {
+export const getHeader = <T extends string | string[] = string>(headers: HeaderLike, key: string): T => {
if (!headers) {
- return asArray ? [] : '';
</file context>
| export const getHeader = <T extends string | string[] = string>(headers: HeaderLike, key: string): T => { | |
| if (!headers) { | |
| return '' as T; | |
| } | |
| if (headers instanceof Headers) { | |
| return (headers.get(key) ?? '') as T; | |
| } | |
| return (headers[key] ?? '') as T; | |
| }; | |
| export function getHeader(headers: HeaderLike | undefined, key: string): string; | |
| export function getHeader(headers: HeaderLike | undefined, key: string, asArray: true): string[]; | |
| export function getHeader(headers: HeaderLike | undefined, key: string, asArray = false): string | string[] { | |
| if (!headers) { | |
| return asArray ? [] : ''; | |
| } | |
| const value = headers instanceof Headers ? (headers.get(key) ?? undefined) : headers[key]; | |
| if (Array.isArray(value)) { | |
| return asArray ? value : (value[0] ?? ''); | |
| } | |
| if (typeof value === 'string') { | |
| return asArray ? [value] : value; | |
| } | |
| return asArray ? [] : ''; | |
| } |
| const { resume } = methodArguments.find((arg) => 'resume' in arg) ?? {}; | ||
| const loginToken = resume ? hashLoginToken(resume) : ''; | ||
| const instanceId = InstanceStatus.id(); | ||
| const clientAddress = info.connection.clientAddress ?? getHeader(httpHeaders, 'x-real-ip'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: clientAddress is a required string, so it may be '' when unknown. Using ?? won’t fall back to x-real-ip in that case (behavior change from the previous getClientAddress helper which used ||). Use || to preserve the fallback when clientAddress is an empty string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/hooks/sauMonitorHooks.ts, line 24:
<comment>`clientAddress` is a required `string`, so it may be `''` when unknown. Using `??` won’t fall back to `x-real-ip` in that case (behavior change from the previous `getClientAddress` helper which used `||`). Use `||` to preserve the fallback when `clientAddress` is an empty string.</comment>
<file context>
@@ -22,7 +21,7 @@ Accounts.onLogin((info: ILoginAttempt) => {
const loginToken = resume ? hashLoginToken(resume) : '';
const instanceId = InstanceStatus.id();
- const clientAddress = getClientAddress(info.connection);
+ const clientAddress = info.connection.clientAddress ?? getHeader(httpHeaders, 'x-real-ip');
const userAgent = getHeader(httpHeaders, 'user-agent');
const host = getHeader(httpHeaders, 'host');
</file context>
| const clientAddress = info.connection.clientAddress ?? getHeader(httpHeaders, 'x-real-ip'); | |
| const clientAddress = info.connection.clientAddress || getHeader(httpHeaders, 'x-real-ip'); |
Proposed changes (including videos or screenshots)
After some investigation, context gathering, and a discussion with Diego Sampaio, we aligned on the scope of this task.
The goal is to improve how DDP connection headers are handled in sauMonitorHooks.ts.
While the broader issue involves multiple places where the full connection object is passed downstream instead of only the required data, addressing all of those cases is outside the scope of this task.
Changes:
Keeping the object construction at the call site improves traceability of fields (e.g. loginToken, instanceId, httpHeaders) and reduces unnecessary indirection.
Issue(s)
CORE-1529 - Improve DDP header handling
Steps to test or reproduce
Further comments
Please keep in mind that this PR touches sensitive code, such as sessions and login/logout.
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.